Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve legibility of registration links #5089 [TASK-1058] #5090

Merged
merged 11 commits into from
Oct 15, 2024
Merged

Improve legibility of registration links #5089 [TASK-1058] #5090

merged 11 commits into from
Oct 15, 2024

Conversation

tinok
Copy link
Member

@tinok tinok commented Aug 30, 2024 •
edited by p2edwards
Loading

Checklist

  1. If you've added code that should be tested, add tests
  2. If you've changed APIs, update (or create!) the documentation
  3. Ensure the tests pass
  4. Make sure that your code lints and that you've followed our coding style
  5. Write a title and, if necessary, a description of your work suitable for publishing in our release notes
  6. Mention any related issues in this repository (as #ISSUE) and in other repositories (as kobotoolbox/other#ISSUE)
  7. Open an issue in the docs if there are UI/UX changes

Description

Move Terms of Service and Privacy Policy links inside the darker area to provide better visibility.

Notes

Some changes in the UI were made based on this discussion in Zulip.

Production Move links into dark shape
and add separator
image
(before this PR)		 image
(before contrast revisions below)

Testing

  • Test wide and narrow layouts for signup page
  • Test with and without links configured in Constance settings
  • Test with and without SSO
No Links With Links
Login ✅🖼️ ✅🖼️
+ SSO ✅🖼️ ✅🖼️
Signup ✅ ✅
+ SSO ✅ ✅🖼️
+ Narrow ✅ ✅
+ SSO+Nar. ✅ ✅🖼️
SSO login ✅🖼️ ✅🖼️
SSO signup ✅ ✅
  • I included some screenshots of how this ended up; the rest of the screens look consistent with the ones shown. — phil

Basic Login Screenshots

Without Links With Links
Login Screenshot_20241011_211321 Screenshot_20241011_211459-1
+ SSO Screenshot_20241011_211305 Screenshot_20241011_211251

Signup Screenshots

Narrow Wide
Screenshot_20241011_211146 Screenshot_20241011_211123-1

SSO login

Without Links With Links
Screenshot_20241011_193037 Screenshot_20241011_193419

Related issues

Fixes #5089

@notion-workspace Notion Workspace
Copy link

Move Privace Policy and Terms of Service into the black area on login screen

@pauloamorimbr pauloamorimbr changed the base branch from main to beta September 23, 2024 14:58
@pauloamorimbr pauloamorimbr changed the title 5089 fix registration links [TASK-1058] Fix registration links #5089 Sep 23, 2024
@pauloamorimbr pauloamorimbr self-assigned this Sep 26, 2024
@pauloamorimbr pauloamorimbr marked this pull request as ready for review September 26, 2024 14:27
@Akuukis Akuukis deleted the branch main October 1, 2024 17:16
@Akuukis Akuukis closed this Oct 1, 2024
@Akuukis Akuukis reopened this Oct 1, 2024
@Akuukis Akuukis changed the base branch from beta to main October 1, 2024 17:28
p2edwards
p2edwards reviewed Oct 7, 2024
View reviewed changes
Copy link
Contributor

@p2edwards p2edwards left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

@pauloamorimbr During review / edge case testing I made some suggested changes in a separate branch (https://github.com/kobotoolbox/kpi/tree/5089-fix-registration-links--additions):

  • Checked what it looks like when an SSO provider is configured — I color-matched the SSO separator with this new links one (went with the new color)
  • Fixed a layout issue on signup page on narrow screen width
    • Also fixed an unrelated color issue that was introduced recently — changed 'required' asterisks back to light red
  • Remove extra separator if the Django admin didn't configure Privacy Policy / Terms links at all

Compare: https://github.com/kobotoolbox/kpi/compare/5089-fix-registration-links..5089-fix-registration-links--additions — (you'd need to merge 'main' into '5089-fix-registration-links' first to see a clean diff)

+sso
image Pasted image 20241004211441

Other questions (not included above)

  • 🎨 @tesster7 What do you think of these legal link colors/sizes/etc.? I wonder if we'd want them to match more closely with the Create Account / Forgot password or not. The contrast is still a little low imo but maybe want to avoid it being too busy.

sso-specific pages

  • 🔧 @pauloamorimbr There are 2 more SSO-related templates (under kpi/kobo/apps/accounts/templates/socialaccount/ — login.html and signup.html) @tinok should we put the Terms / Privacy links here too, or is that not necessary? (the deep signup / login pages that you get if you're using SSO.)
sso-specific login page e.g. with legal links
image Pasted image 20241004211246

kobo/apps/accounts/templates/account/login.html Outdated Show resolved Hide resolved
@pauloamorimbr
Copy link
Contributor

@p2edwards thank you for all the inputs.

I believe I addressed all the comments, and I've split the legals in a separated file so it could be included in the needed places.

p2edwards
p2edwards approved these changes Oct 12, 2024
View reviewed changes
Copy link
Contributor

@p2edwards p2edwards left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 ✅ Ready to merge @pauloamorimbr — if you're cool with the pushed commits:

  • I finished setting up SSO on my dev machine so I could check how signup looks.
    • I adjusted it to match the others better, that's the only new thing
  • Added a couple more fixes from the review branch (that got lost in the rebase I think)
    • I added screenshots in comments for this PR to make these easier to review
  • Edited description with test plan and screenshots of how things look now
    • This change seemed simple. But with the SSO option, a responsive signup layout, and the Constance config options, it affected 4 templates and had at least 16 distinct variants. I listed them in the description in case we have to test something like this again.

jsapp/scss/stylesheets/partials/_registration.scss
.registration__legal {
margin-top: 10px;
margin-bottom: -20px;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one's for consistency with the rest of the views

SSO no links SSO unadjusted SSO adjusted
sso-signup-no-links sso-signup-before sso-signup-adjusted

jsapp/scss/stylesheets/partials/_registration.scss
.registration__footer {
clear: both;
text-align: center;
margin: 20px 10px 10px 10px;
margin: 30px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this 30px margin to prevent an overlap between the background shape and the photo credit:

before after
Screenshot_20241011_185542-1 Screenshot_20241011_190634

jsapp/scss/stylesheets/partials/_registration.scss
Comment on lines +451 to +454
.registration__legal {
order: 4;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes this problem:

before after (top) after (bottom)
Screenshot_20241011_190610 Screenshot_20241011_205819 Screenshot_20241011_205712-1

kobo/apps/accounts/templates/legal/registration_legal.html Show resolved Hide resolved
kobo/apps/accounts/templates/account/signup.html Outdated
@@ -91,6 +91,5 @@ <h2>{% trans "Register using SSO" %}</h2>
</div>
<div style="clear:both;"></div>
{% include "../legal/registration_legal.html" with config=config %}
</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An extra </div> snuck in here (I removed it)

Was causing the photo credit to do this:

before after
Screenshot_20241011_185218 Screenshot_20241011_210602

@p2edwards p2edwards changed the title [TASK-1058] Fix registration links #5089 Improve legibility of registration links #5089 [TASK-1058] Oct 12, 2024
@p2edwards p2edwards added the UI & UX User interface problems and improvements label Oct 15, 2024
pauloamorimbr
pauloamorimbr approved these changes Oct 15, 2024
View reviewed changes
Copy link
Contributor

@pauloamorimbr pauloamorimbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great fixes.

@pauloamorimbr pauloamorimbr merged commit b646a1d into main Oct 15, 2024
8 checks passed
@pauloamorimbr pauloamorimbr deleted the 5089-fix-registration-links branch October 15, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pauloamorimbr pauloamorimbr pauloamorimbr approved these changes

@p2edwards p2edwards p2edwards approved these changes

Assignees

@pauloamorimbr pauloamorimbr

Labels
Front end UI & UX User interface problems and improvements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make TOS and Privacy Policy links more legible on light backgrounds for registration page
4 participants
@tinok @pauloamorimbr @p2edwards @Akuukis